Skip to content

feat(github): protect vaulted tags against deletion and force-push - #7

Merged
petrus-v merged 2 commits into
petrus-v:mainfrom
bosd:feat/protect-vault-tags
Oct 24, 2026
Merged

petrus-v merged 2 commits into
petrus-v:mainfrom
bosd:feat/protect-vault-tags

Conversation

@bosd

@bosd bosd commented Oct 1, 2026 •

Copy link
Copy Markdown

Closes part of #6 — the "start simple with support at fork time" piece you
suggested.

Why

The README's first promise is that uvault "pushes them as immutable tags".
Nothing made them immutable: a tag can be deleted or force-moved by anyone with
push access to the vault.
That is the same failure mode vaulting exists to prevent, relocated from upstream to the vault.

What

GitHubForge.ensure_tag_ruleset(repo_path) applies a tag ruleset blocking
deletion and non_fast_forward, and fork() calls it after a successful fork.

It takes a repository path rather than deriving one from the source, so the
same primitive serves a per-upstream fork and a single shared vault repo.
That was deliberate given the direction you raised in #6 — nothing here has to be rewritten if the single-repo layout wins, and a protect-tags subcommand would reuse this unchanged across whatever repositories the config names.

Three properties, each with a test

creation is never blocked. It is the obvious third rule to add and it
breaks every later sync, because the vault has to keep accepting new tags. Only
deletion and non_fast_forward.

It never raises. Creating a ruleset needs Administration: write, strictly
more than forking and pushing, so a token that cannot do it must still be able to
vault. 403 and 404 get specific messages — 404 being what a plan without rulesets
looks like — and anything else is warned about and swallowed. fork() therefore
still returns True when the tags could not be protected, which also covers the
opt-out case you mentioned.

That one is a real guard rather than a theoretical one: an earlier draft raised
ValueError on an unexpected response shape and propagated it out of fork(), failing the fork itself. There is a regression test for it.

It is idempotent. An existing ruleset with the same name and target is left alone, so re-running costs one GET.

Notes

  • PyGithub 2.9.1 has no ruleset API, so this goes through
    Github.requester.requestJsonAndCheck. No private attributes.
  • The ruleset name is a class attribute (uvault-tags-immutable) so the
    idempotency check and any future protect-tags agree on it.
  • Coverage stays at 100% in the gating --extra github run (142 passed).

Not included, as discussed: protect-tags itself, and anything presuming a
topology. Happy to follow up with the subcommand once #6 settles, or sooner if
you would rather have it now.

@codecov-commenter

codecov-commenter commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (72c78ba) to head (9a3f14b).

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #7   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines         1074      1105   +31     
=========================================
+ Hits          1074      1105   +31     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bosd
bosd force-pushed the feat/protect-vault-tags branch from b4eaac1 to b9f1127 Compare October 1, 2026 16:05
The README's first promise is that uvault "pushes them as immutable tags",
but nothing made them immutable: a tag can be deleted or force-moved by
anyone with push access to the vault, which is the same failure mode
vaulting exists to prevent, just relocated from upstream to the vault.

Adds `GitHubForge.ensure_tag_ruleset(repo_path)`, which applies a tag
ruleset blocking `deletion` and `non_fast_forward`, and calls it after a
successful fork.

Deliberately takes a repository path rather than deriving one from the
source, so the same primitive serves a per-upstream fork and a single
shared vault repository. That keeps it useful whichever topology the
discussion in petrus-v#6 settles on, and it is the piece a `protect-tags`
subcommand would reuse unchanged.

Three properties that matter, each with a test:

- **`creation` is never blocked.** It is the obvious third rule and it
  would break every later sync, because the vault has to keep accepting
  new tags. Only `deletion` and `non_fast_forward`.
- **It never raises.** Creating a ruleset needs `Administration: write`,
  strictly more than forking and pushing, so a token that cannot do it
  must still be able to vault. 403 and 404 get specific messages -- 404
  being what a plan without rulesets looks like -- and anything else is
  warned about and swallowed. `fork()` therefore still succeeds when the
  tags cannot be protected.
- **It is idempotent.** An existing ruleset with the same name and target
  is left alone, so re-running costs one GET.

Coverage stays at 100% in the gating `--extra github` run.

@petrus-v petrus-v left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you consider adding some reference documentation in uvault sync to explain that a uvault-tags-immutable ruleset will be automatically setup on the vault repository ?

Comment thread src/uvault/github.py Resolved
Review found that `non_fast_forward` alone does not make a tag immutable:
force-pushing over an existing tag still succeeded. Tested on a real
repository, the `update` restriction is what prevents it, so the ruleset
now carries `deletion`, `non_fast_forward` and `update`.

Blocking updates costs nothing here. uvault pushes
`<sha>:refs/tags/<tag>` with no `+` refspec and no `--force` (vcs.py
`vault_tag` / `vault_release_tag`), so it never moves a tag that already
exists. `creation` remains deliberately unrestricted -- that one really
would break every later sync.

Also adds the requested reference documentation under `uvault sync`,
next to the existing automatic-forking section: what the ruleset is
called, which three rules it applies and why, that creation is left
alone, and that the whole thing is advisory -- a token without
`Administration: write` still vaults, with a warning. Notes the two cases
that surprise people: rulesets need a public repository on GitHub Free,
and repositories forked before this existed are not retrofitted.
@bosd

bosd commented Oct 6, 2026 •

Copy link
Copy Markdown
Author

Thanks for actually testing it, that caught a real hole. I had checked that the
ruleset got created and was enforcing, but I never tried force-pushing over an
existing tag, so I just took non_fast_forward at face value. Your screenshot
shows it isn't enough on its own.

Added update. The ruleset now applies deletion, non_fast_forward and
update.

Restricting updates sounds like it might break re-syncing, so worth saying why it
doesn't. uvault pushes <sha>:refs/tags/<tag> with no + refspec and no
--force (vcs.py, in vault_tag and vault_release_tag), so it never moves a
tag that already exists. And a re-sync at a different commit produces a different
tag name anyway, since the default templates embed {sha}. So nothing legitimate
gets blocked. creation stays unrestricted, that one genuinely would break every
later sync.

One caveat on my side. The tests check the payload shape against a mocked API,
not that GitHub actually accepts update on a tag target. Your UI test is the
better evidence here, so if you get a moment to confirm the API path behaves the
same way, that would close the loop properly.

Reference docs added under uvault sync, right next to the automatic-forking
section. They cover:

  • the ruleset name, the three rules, and why each one is there
  • that creation is deliberately left alone
  • that the whole thing is advisory. A token without Administration: write still
    vaults fine, it just prints a warning, so this can't block anyone
  • two things that surprise people: rulesets need a public repository on GitHub
    Free, and repos forked before this existed don't get retrofitted

That last one is the real gap in this PR. Protection happens at fork time, so
existing vaults stay unprotected until something re-applies it. That's what
protect-tags would be for, and I'm happy to follow up with it whichever way the
single-repo discussion in #6 lands, since ensure_tag_ruleset takes a repository
path and doesn't care about the topology.

zensical build is clean and CI is green across the matrix.

@petrus-v
petrus-v merged commit 8569124 into petrus-v:main Oct 24, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants